Repository navigation
fix(core): skip counting bytes past the last newline - #61449
andreivince wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good. I ran the reader on the 64 MiB single-line file from #61448 with limit: 2. On the base it took about 5.7 s and 685 MiB of memory. With this change it took 89 ms and 226 MiB, and the returned page was the same. I also compared both versions on 300 random files. They mixed very long lines that span several chunks, multi-byte and cut-off UTF-8 characters, BOMs, CRLF line endings and NUL bytes, with random offsets and limits. Every page and error matched the base exactly, including the line counts in out-of-range errors and the old rule that strips a BOM from the selected line. The reader tests pass and typecheck:tsc passes.
The public API is unchanged. Only internal helpers were removed and an internal type narrowed.
I have no blocking concerns.
andreivince
changed the title
There was a problem hiding this comment.
The new commits look correct. Counting 1 + newlines before lastIndexOf(10) gives the same total as counting the whole chunk, and chunks with no newline are now skipped.
On the #61448 script with a 64 MiB file, the base (which already has #63030) took about 172–175 ms and this change took 82–90 ms. Memory and the number of chunk reads were the same. I also compared both versions on 300 random files, about half of them read from the end with negative offsets. They included long lines spanning several chunks, multi-byte characters, BOMs, CRLF line endings and NUL bytes. Every page and error matched the base. The reader tests pass and typecheck:tsc passes.
Note that the slowdown and memory growth described in #61448 no longer reproduce on the base. #63030 already fixed them, so this change is a speedup on top of that.
The public API is unchanged. I have no concerns.
There was a problem hiding this comment.
The only new commit merges dev into the branch and leaves read-filesystem.ts unchanged, so the change is the same one I reviewed before. On the new head, the reader tests still pass and the #61448 64 MiB script returns the same output in about 58 ms. The public API is unchanged. I have no concerns.
Issue for this PR
Closes #61448. Follow-up to #63030.
Type of change
What does this PR do?
After #63030, the reader still goes through every byte in a chunk to count newlines. Moving the existing last newline lookup before that loop lets it skip the count in chunks with no newline and stop at the last newline in the rest.
On my Mac, the 64 MiB repro from #61448 took about 98 ms on dev
2f886a2a5eand 43 ms with this patch, over 12 runs each.How did you verify your code works?
The reader tests passed on macOS, Linux, and Windows. I also checked the long line, continuation, and both forms of tail read through the CLI on all three. The full Windows Core run hit a timeout in a Git packing test.
Screenshots / recordings
Not applicable.
Checklist